Add section styles switch button in block toolbar in zoom out mode #67140
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
matiasbenedetto
added
[Type] Enhancement
|
Size Change: +463 B (+0.03%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
I'm not feeling the popover + popovers. Perhaps we just iterate through style variations when you select the control? to not further block the content?
|
|
May be nice to use the current style's background color in the control, like this:
|
That seems to be pretty specific to variations with background colors specified and the variation can be any style. Could it be confusing to change that color when using a variation with a background color and not when using variations changing other styles/settings? |
No, I don't think it'd be confusing if it does have a background color and it's used here. |
Do you suggest removing this part entirely?
|
There was a problem hiding this comment.
Thanks for working on this one @matiasbenedetto 👍
I'm not feeling the popover + popovers. Perhaps we just iterate through style variations when you select the control? to not further block the content?
+1. The current experience gets worse on smaller devices as well.
The proposed simplification would also likely make it easier to ensure the control is accessible.
On that note, can you update the PR description with complete test instructions? People with visual impairments might also like to review the PR. Directing them to watch an inaccessible video isn't great.
Do you suggest removing this part entirely?
That's my understanding. Clicking on the section styles button would move to the next section style, looping back to the default once the list is exhausted.
It would probably mean we can't reuse BlockStyles here and need to do something similar to the useStylesForBlock hook's onSelect. That should be fairly straightforward though with replaceActiveStyle util and updateBlockAttributes action doing the heavy lifting. The real work would be in tracking which block style we're up to.
If the suggestion is indeed to add a |
What were those reasons?
You mean sorta like the current state of this PR? Maybe just without the preview popover? |
They were too random, also a user would need to rotate all of them to go back to one they liked, etc..
I mean like this: 
|
I don't think the "too random" issue is a problem with the section styles being rotated. I can agree that having to rotate all section styles to get back to the default is a bit annoying though.
I see what the "Change design" list is. I was trying to understand what the suggestion was here for section styles. Is the suggestion that the list contains previews of the block with the different section styles applied? So, no list of buttons like this PR currently has? If I'm now following you correctly, sounds like it's worth a shot. For discussion purposes in the meantime, here's a quick diff that can be applied to see what cycling section styles through a single button would feel like. Diff for section styles toolbar button to cycle stylesNote: This is just a quick hack so it uses a quick ad hoc SVG icon so that the fill within the teardrop path can be set with the current section style's background color. diff --git a/packages/block-editor/src/components/block-toolbar/index.js b/packages/block-editor/src/components/block-toolbar/index.js
index ea068d81265..817cfea761d 100644
--- a/packages/block-editor/src/components/block-toolbar/index.js
+++ b/packages/block-editor/src/components/block-toolbar/index.js
@@ -226,7 +226,10 @@ export function PrivateBlockToolbar( {
<ChangeDesign clientId={ blockClientIds[ 0 ] } />
) }
{ showSwitchSectionStyleButton && (
- <SwitchSectionStyle clientId={ blockClientIds[ 0 ] } />
+ <SwitchSectionStyle
+ clientId={ blockClientIds[ 0 ] }
+ blockName={ blockType?.name }
+ />
) }
{ shouldShowVisualToolbar && showSlots && (
<>
diff --git a/packages/block-editor/src/components/block-toolbar/switch-section-style.js b/packages/block-editor/src/components/block-toolbar/switch-section-style.js
index 8e4b9c0b74b..4101b85d3fa 100644
--- a/packages/block-editor/src/components/block-toolbar/switch-section-style.js
+++ b/packages/block-editor/src/components/block-toolbar/switch-section-style.js
@@ -4,55 +4,110 @@
import {
ToolbarButton,
ToolbarGroup,
- Dropdown,
- __experimentalDropdownContentWrapper as DropdownContentWrapper,
Icon,
+ Path,
+ SVG,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';
-import { color } from '@wordpress/icons';
+import { useDispatch, useSelect } from '@wordpress/data';
+import { useContext } from '@wordpress/element';
/**
* Internal dependencies
*/
-import BlockStyles from '../block-styles';
import useStylesForBlocks from '../block-styles/use-styles-for-block';
+import { replaceActiveStyle } from '../block-styles/utils';
+import { store as blockEditorStore } from '../../store';
+import { GlobalStylesContext } from '../global-styles';
+import { globalStylesDataKey } from '../../store/private-keys';
+import { getVariationStylesWithRefValues } from '../../hooks/block-style-variation';
-const POPOVER_PROPS = {
- placement: 'bottom-start',
-};
+const styleIcon = (
+ <SVG
+ viewBox="0 0 24 24"
+ xmlns="http://www.w3.org/2000/svg"
+ width="24"
+ height="24"
+ aria-hidden="true"
+ focusable="false"
+ >
+ <Path
+ stroke="currentColor"
+ strokeWidth="1"
+ d="M17.2 10.9c-.5-1-1.2-2.1-2.1-3.2-.6-.9-1.3-1.7-2.1-2.6L12 4l-1 1.1c-.6.9-1.3 1.7-2 2.6-.8 1.2-1.5 2.3-2 3.2-.6 1.2-1 2.2-1 3 0 3.4 2.7 6.1 6.1 6.1s6.1-2.7 6.1-6.1c0-.8-.3-1.8-1-3z"
+ />
+ </SVG>
+);
function SwitchSectionStyle( { clientId } ) {
- const { stylesToRender } = useStylesForBlocks( { clientId } );
+ const { stylesToRender, activeStyle, className } = useStylesForBlocks( {
+ clientId,
+ } );
+ const { updateBlockAttributes } = useDispatch( blockEditorStore );
+
+ // Get global styles data
+ const { merged: mergedConfig } = useContext( GlobalStylesContext );
+ const { globalSettings, globalStyles, blockName } = useSelect(
+ ( select ) => {
+ const settings = select( blockEditorStore ).getSettings();
+ return {
+ globalSettings: settings.__experimentalFeatures,
+ globalStyles: settings[ globalStylesDataKey ],
+ blockName: select( blockEditorStore ).getBlockName( clientId ),
+ };
+ },
+ [ clientId ]
+ );
+
+ // Get the background color for the active style
+ const activeStyleBackground = activeStyle?.name
+ ? getVariationStylesWithRefValues(
+ {
+ settings: mergedConfig?.settings ?? globalSettings,
+ styles: mergedConfig?.styles ?? globalStyles,
+ },
+ blockName,
+ activeStyle.name
+ )?.color?.background
+ : undefined;
if ( ! stylesToRender || stylesToRender.length === 0 ) {
return null;
}
+ const handleStyleSwitch = () => {
+ const currentIndex = stylesToRender.findIndex(
+ ( style ) => style.name === activeStyle.name
+ );
+
+ const nextIndex = ( currentIndex + 1 ) % stylesToRender.length;
+ const nextStyle = stylesToRender[ nextIndex ];
+
+ const styleClassName = replaceActiveStyle(
+ className,
+ activeStyle,
+ nextStyle
+ );
+
+ updateBlockAttributes( clientId, {
+ className: styleClassName,
+ } );
+ };
+
return (
- <Dropdown
- popoverProps={ POPOVER_PROPS }
- renderToggle={ ( { onToggle, isOpen } ) => {
- return (
- <ToolbarGroup>
- <ToolbarButton
- onClick={ () => onToggle( ! isOpen ) }
- aria-expanded={ isOpen }
- label={ __( 'Styles' ) }
- >
- <Icon icon={ color } />
- </ToolbarButton>
- </ToolbarGroup>
- );
- } }
- renderContent={ () => (
- <DropdownContentWrapper
- className="block-editor-block-toolbar-change-design-content-wrapper"
- paddingSize="none"
- >
- <BlockStyles clientId={ clientId } />
- </DropdownContentWrapper>
- ) }
- />
+ <ToolbarGroup>
+ <ToolbarButton
+ onClick={ handleStyleSwitch }
+ label={ __( 'Switch style' ) }
+ >
+ <Icon
+ icon={ styleIcon }
+ style={ {
+ fill: activeStyleBackground || 'transparent',
+ } }
+ />
+ </ToolbarButton>
+ </ToolbarGroup>
);
}
Screen.Recording.2024-11-20.at.4.39.55.pm.mp4 |
Sorry for not being clear enough. Yes, exactly that. |
I think we start here first, then explore a similar pattern to "Change design", but with block previews of the actual pattern. |
|
I committed the diff proposed by @aaronrobertshaw (Thanks!)
In that case, I think the PR is good to go. We can implement the list of styles applied to the pattern as a follow-up. |
There was a problem hiding this comment.
I think we start here first, then explore a similar pattern to "Change design", but with block previews of the actual pattern.
Nothing stopping us iterating in follow-ups, so no issues with that from my end 👍
See the screencast below.
As per my last review, can we please add proper test instructions?
I committed the diff proposed by @aaronrobertshaw (Thanks!)
You're welcome but that diff wasn't meant as "production ready" code though 😅
From my testing, it works well in both the site and post editors. I did get caught out a few times trying to click on the section styles button only for the delayed "Change design" button to pop into the toolbar and shift things under my cursor. That would be nice to smooth out, if possible, in a separate follow-up.
The main aspect of the diff that was only for demo purposes was the temporary SVG used as the icon.
The colors icon defines a path that creates the outline. It uses fill to color that outline. This is why I couldn't apply a background color to it.
The temporary SVG in my diff redefines that path to be the teardrop shape only and uses stroke for creating the outline. While this allows the use of fill to apply the background color to the teardrop shape, it does mean there's potential for a slight difference in width between the stroke based outline and the fill based approach in the original icon.
That might be a blocker to using this temporary SVG as a drop-in replacement for the icon already in the icon library. If setting the background color within the icon's shape is something we might wish to do elsewhere it could be good to move the icon into the library perhaps as colors-fill or something?
Back to the stroke width, perhaps our design gurus could shed some light on what the exact stroke width should be. It might then be possible to enforce that as well?
Instructions and video updated in the PR's body.
I think the stroke should not be defined. At least, that's what I observed in other filled library icons. Examples: I submitted a PR adding the icon to the library:
I think the icon should not stop us to merge this. We can replace it in a follow-up PR with the library version if #67202 gets merged. |
|
Thanks for the iterations @matiasbenedetto 🚀 If @richtabor and our design gurus are happy with the interim icon, I'm ok with moving forward as is. That said, it looks like #67202 already has a code review approval. If it is that close we might as well push that over the line first and tweak this before merging. I don't think it holds up landing this greatly. |
|
Tweaked the label and strokeWidth. CleanShot.2024-11-22.at.09.34.23.mp4 |
|
Flaky tests detected in 339b1c5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11974623634
|
What?
Add section styles switch button in block toolbar in zoom-out mode.
Why?
Fixes: #66465
How?
Adding the button to the toolbar.
Testing Instructions
Screenshots or screencast
Screencast.from.2024-11-21.08-41-31.mp4